fix: handle queries on non-existing table gracefully #3869
Conversation
taimoorzaeem
force-pushed
the
non-existing-table/issue3697
branch
from
95d4973 to
52591c0
Compare
|
Now that we rely on the schema cache for simple table filters, we need to make sure these requests are fast when a schema cache load for relationships is slow (#3046). There's a test that validates the above behavior (a request with resource embedding is blocked until the schema cache finishes loading): postgrest/test/io/test_big_schema.py Lines 10 to 34 in 52591c0 We need an additional test that asserts that a table request (with no resource embedding) finishes in a short |
taimoorzaeem
force-pushed
the
non-existing-table/issue3697
branch
from
f008e21 to
bdaf6bf
Compare
|
One last concern, we are still using postgrest/src/PostgREST/Error.hs Line 135 in e2dd435 This seems fine for now, but maybe should be improved later (add error msg, details etc) for better UX? |
taimoorzaeem
force-pushed
the
non-existing-table/issue3697
branch
3 times, most recently
from
8d6bb71 to
bb4f692
Compare
taimoorzaeem
force-pushed
the
non-existing-table/issue3697
branch
from
bb4f692 to
7196aa5
Compare
|
@steve-chavez Could this get a review? Seems good to me. |
* Add new `TableNotFound` error with error code PGRST205 * Closes PostgREST#3697, PostgREST#3602.
taimoorzaeem
force-pushed
the
non-existing-table/issue3697
branch
from
7196aa5 to
0b52035
Compare
| @@ -253,6 +255,12 @@ instance JSON.ToJSON ApiRequestError where | |||
| toJSON (ColumnNotFound relName colName) = toJsonPgrstError | |||
| SchemaCacheErrorCode04 ("Could not find the '" <> colName <> "' column of '" <> relName <> "' in the schema cache") Nothing Nothing | |||
|
|
|||
| toJSON (TableNotFound schemaName relName tbls) = toJsonPgrstError | |||
| SchemaCacheErrorCode05 | |||
| ("Could not find relation '" <> schemaName <> "." <> relName <> "' in the schema cache") | |||
There was a problem hiding this comment.
The "relation" term might be confused with "relationship". I think we can be more precise here and instead refer to "table" or "view", we have a tableIsView :: Bool field already.
There was a problem hiding this comment.
Although it doesn't make sense to discriminate table or view if we don't know what we're looking for. Maybe you could omit the "relation" term altogether, or change it to "table or view".
| -- Do a fuzzy search in all tables in the same schema and return closest result | ||
| tableNotFoundHint :: Text -> Text -> [Table] -> Maybe Text | ||
| tableNotFoundHint schema tblName tblList | ||
| = fmap (\tbl -> "Perhaps you meant the relation '" <> schema <> "." <> tbl <> "'") perhapsTable |
| readPlan :: Bool -> QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> Either Error ReadPlanTree | ||
| readPlan fromCallPlan qi@QualifiedIdentifier{..} AppConfig{configDbMaxRows, configDbAggregates} SchemaCache{dbTables, dbRelationships, dbRepresentations} apiRequest = |
There was a problem hiding this comment.
This Bool here is confusing, looks like it breaks the abstraction. Is there another way to do this?
Let's try and do it correctly now, even if it requires a refactor. Otherwise tech debt accrues and refactors become more laborious later.
There was a problem hiding this comment.
Let's try and do it correctly now, even if it requires a refactor. Otherwise tech debt accrues and refactors become more laborious later.
Agreed.
Add new
TableNotFounderror and add json errormessage for this error. Closes #3697, closes #3602.
(Couldn't think of a better changelog/commit message. Feel free to suggest a new/better message).